New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consolidate JS, TS, LiveScript, CoffeeScript on common foundation #2518
Consolidate JS, TS, LiveScript, CoffeeScript on common foundation #2518
Conversation
daf9365
to
87bd22b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea.
I didn't check all the keywords though, hope you did it right.
src/languages/bash.js
Outdated
className: 'meta', | ||
begin: /^#![^\n]+sh\s*$/, | ||
const SHEBANG = hljs.SHEBANG({ | ||
binary: /(fish|bash|zsh|sh)/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more... What's wrong with previous .+sh
approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the relevance would have to change.. not every binary ending in sh
is a shell... If we went a high relevance then we should have an explicit list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what are the most frequently used shells (except for bash and historically sh, of course). Probably your list is good enough, I just don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like to suggest a few more to add, we have the room. :-) I'd rather add them slowly than have false positives. And if your concern is just highlighting the shebang itself (vs treating it as a comment) we could add a basic SHEBANG rule also with 0 relevancy - to catch shell like files with an unknown binary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look at that as a "match any *sh" I read that as match the path then sh
... but you're right that before it would have been more generic. I guess I just dislike that - at least when it comes to appointing 10 relevancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like to suggest a few more to add, we have the room. :-)
I've heard of, say, ksh
, but I have no idea if someone use it these days. I have no must-have suggestions.
And if your concern is just highlighting the shebang itself (vs treating it as a comment) we could add a basic SHEBANG rule also with 0 relevancy - to catch shell like files with an unknown binary...
I think it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expanded the list:
"fish",
"bash",
"zsh",
"sh",
"csh",
"ksh",
"tcsh",
"dash",
"scsh",
87bd22b
to
edaefb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me now.
Builds a common keyword foundation for any grammar that is building on top of JavaScript: - LiveScript - CoffeeScript - TypeScript Also uses this common foundation for JS itself.
f40d942
to
a6c1843
Compare
Just the beginning, but it's a start.